Pull Request Review: Payment System Implementation

Overview

This PR introduces a comprehensive payment system integration with TossPayments, including order management, cart improvements, and monitoring infrastructure. The changes span 37 files with significant refactoring of payment and order flows.

Code Quality & Best Practices ✅

Strengths:

1. DTO Pattern: Excellent use of DTOs (CheckoutItemDto, CreateOrderRequestDto, PaymentConfirmRequestDto) to separate API contracts from domain models
2. Service Layer Design: Clean separation of concerns with dedicated services for orders, payments, and cart management
3. Builder Pattern: Consistent use of Lombok builders throughout entity creation
4. Path Consolidation: Great cleanup in CartController.java - moving from /cart/add to @RequestMapping("/cart") + /add is cleaner
5. Immutability: CheckoutItemDto uses final fields, preventing accidental mutations

Areas for Improvement:

1. Spring Boot Version Downgrade ⚠️
build.gradle:3

- id 'org.springframework.boot' version '3.5.5'
+ id 'org.springframework.boot' version '3.2.5'

Issue: Downgrading from 3.5.5 to 3.2.5 loses bug fixes and security patches.
Recommendation: If this was necessary for compatibility, document why. Otherwise, keep the latest stable version.

2. Missing @ResponseBody in Critical Endpoint 🐛
PaymentController.java:49

@PostMapping("/confirm/payment")
public ResponseEntity<JSONObject> confirmPayment(...)

Issue: The @ResponseBody annotation was removed. While ResponseEntity implies a response body, explicit annotation is clearer for REST APIs.
Recommendation: Add @ResponseBody back for consistency with other endpoints.

Security Concerns 🔒

Critical Issues:

1. Payment Amount Validation ⚠️
PaymentController.java:58-61

if (!order.getTotalAmount().equals(requestDto.getAmount().intValue())) {
    throw new RuntimeException("주문 금액이 일치하지 않습니다.");
}

Good: Amount validation prevents price manipulation
Issue: Error doesn't trigger payment cancellation with TossPayments API
Recommendation: Implement the TODO comment on line 88 of OrderService.java:

// TODO: 금액 위변조 시도가 의심되므로 결제 취소 API를 호출하는 로직 추가 필요

2. Input Validation Missing
DTOs lack validation annotations:

@Getter @Setter
public class PaymentConfirmRequestDto {
    private String paymentKey;  // Should be @NotBlank
    private String orderId;     // Should be @NotBlank
    private Long amount;        // Should be @NotNull @Positive
}

Recommendation: Add Jakarta Bean Validation:

@NotBlank(message = "결제 키는 필수입니다")
private String paymentKey;

@NotNull @Positive
private Long amount;

3. API Key Exposure Risk
PaymentController.java:40

@Value("${toss.secret.key}")
private String API_SECRET_KEY;

Good: Using externalized configuration
Concern: Ensure application.properties is in .gitignore and keys are managed via environment variables in production

Potential Bugs 🐛

1. Race Condition in Order Creation ⚠️
OrderService.java:38-43

for (CartItem cartItem : cartItems) {
    if (cartItem.getProduct().getStock() < cartItem.getQuantity()) {
        throw new RuntimeException("재고가 부족한 상품이 있습니다: " + cartItem.getProduct().getName());
    }
}
// ... later, stock is decreased in confirmPayment()

Issue: Stock is checked but NOT reserved. Between createOrderFromCart() and confirmPayment(), stock could be sold to others.
Scenario:

1. User A creates order (stock=1, check passes)
2. User B creates order (stock=1, check passes)
3. User A confirms payment (stock becomes 0)
4. User B confirms payment (stock becomes -1!) ❌

Recommendation: Use optimistic locking (already implemented in Product.java:25) or lock stock immediately in createOrderFromCart().

2. Unchecked Type Cast
OrderController.java:77 (in diff, removed code was problematic)
The new code using CreateOrderRequestDto fixes the previous unsafe cast - Good refactoring! ✅

3. Cart Item Removal Logic
CartService.java:122-128

cart.getCartItems().removeAll(itemsToRemove);

Concern: This relies on JPA's orphanRemoval=true. Verify Cart entity has:

@OneToMany(mappedBy = "cart", cascade = CascadeType.ALL, orphanRemoval = true)
private List<CartItem> cartItems;

Performance Considerations ⚡

1. N+1 Query Problem - Acknowledged ✅
CartService.java:140-156

// N+1 문제 발생 버전 (테스트용)
Cart cart = cartRepository.findByMemberId(member.getId())

Good: Explicitly documented for testing
Issue: This is in production code!
Recommendation:

• Use the commented-out findByMemberIdWithItems() in production
• Move N+1 test version to a separate test profile or remove it

2. Synchronous Payment Confirmation
PaymentController.java:53-71
The payment flow is entirely synchronous. If TossPayments API is slow, users wait.
Recommendation: Consider async processing with status polling for large-scale deployments.

3. Missing Database Indexes
High-traffic queries need indexes:

• Order.orderId (primary key, already indexed)
• Order.paymentKey (line 133 in OrderService.java - needs index)
• CartItem.cart_id + CartItem.product_id composite index

Test Coverage 📊

Good:

• OrderServiceConcurrencyTest.java tests optimistic locking ✅
• Proper use of @SpringBootTest and CountDownLatch for concurrency

Missing:

1. Payment Controller Tests: No tests for /confirm/payment endpoint
2. DTO Validation Tests: No tests for malformed payment requests
3. Integration Tests: No end-to-end tests for cart → order → payment flow
4. Edge Cases:
   • What happens if TossPayments API returns 500?
   • What if payment takes >2 minutes (timeout)?
   • Duplicate payment confirmation attempts?

Recommendation: Add tests for:

@Test
void shouldRejectDuplicatePaymentConfirmation() { ... }

@Test
void shouldHandleTossPaymentsAPIFailure() { ... }

@Test
void shouldPreventNegativeStockAfterConcurrentOrders() { ... }

Monitoring & Observability ✅

Excellent Additions:

1. Prometheus + Grafana setup in docker-compose.yml
2. Actuator endpoints (/actuator/prometheus)
3. Request logging filter (RequestLoggingFilter.java)

Recommendations:

• Add custom metrics for payment success/failure rates
• Log payment confirmation results with correlation IDs
• Set up alerts for failed payment confirmations

Architecture Improvements 🏗️

What I Like:

1. Two-Phase Order Creation:

   • Phase 1: createOrderFromCart() → PENDING order
   • Phase 2: confirmPayment() → PAID order with stock decrease

   This is correct for PG integration! ✅

2. Clean Controller Refactoring: Removed complex request parsing in favor of DTOs

Suggestions:

1. Add Idempotency: Payment confirmations should be idempotent (check order.status before processing)
2. Webhook Support: TossPayments can send webhooks for payment events - consider adding a webhook endpoint
3. Saga Pattern: For complex multi-step processes (order → payment → stock → cart cleanup), consider implementing saga pattern for rollback on failures

Summary

Approve with Recommendations ✅

This is a solid implementation of a payment system with good separation of concerns and proper DTO usage. The main concerns are:

1. 🔴 Critical: Fix race condition in stock reservation between order creation and payment confirmation
2. 🟡 High: Add input validation to DTOs
3. 🟡 High: Remove N+1 query code from production
4. 🟡 Medium: Implement payment cancellation on amount mismatch
5. 🟢 Low: Add comprehensive tests for payment flows

Security: Generally good, but needs validation annotations
Performance: Acceptable, but watch for N+1 queries
Maintainability: Excellent - clean code with good patterns
Testing: Needs more coverage on payment flows

Great work on the monitoring setup and DTO refactoring! 🚀